-
Notifications
You must be signed in to change notification settings - Fork 485
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add minio support via http #147
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
I signed it! |
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
I signed it! |
CLAs look good, thanks! |
rather than add an entirely new cache, how hard would it be to modify the existing s3 implementation to support minio as well? I've never used minio, so am not sure what additional pieces it really needs. Can you give me an example of a minio URL? I get that endpoint would point to your minio instance, but what value do you use for region in that case? |
As an example of how I was thinking we could modify the existing s3 cache, what if we pushed any of these additional config options we need into the query string? So you could use minio with a URL such as:
Or something like that? Or are there other changes for minio that I overlooked? (And actually, could we safely just enable forcePathStyle all the time, even for AWS? Is there a downside to that?) |
Not a bad shout, the reason I kept it separate was to keep things simple and not to confuse current users with changes, more than happy to take the advice and take a stab at modifying the current s3cache, I am not a heavy user of s3 or minio so I wouldn't like to comment on the forcePathStyle. Let me know your thoughts. |
ok so looking at this a bit more I can see a few issues that I am not sure how to resolve, in the current URL the region is S3's endpoint, changing this behavior is more than likely going to have an impact on current users, I like the idea of using query strings but not sure how this will work. S3: Any advice here would be great. |
I'd also love to have support for minio, this PR seems to be passing everything, can it be merged? |
I would still much rather find a way to make these kinds of s3-compatible services work with the existing s3cache implementation, even if that means we need to make some adjustments to accommodate that. Especially since all this seems to be doing is duplicating the s3cache with just a couple of extra options on the underlying aws.Config struct. I can take a quick stab at what that might look like, but don't have an easy way to test it. I can try setting up a minio instance when I have a chance, but that may not be anytime soon. |
Thanks for the reply, is there any way I can help you with this? Running minio using docker is quite easy. Let me know if I can help somehow |
Could you try testing the "s3compat" branch I just pushed and see if that works? https://github.com/willnorris/imageproxy/tree/s3compat |
@willnorris this is working perfectly for me, testing with Minio. |
Awesome, thanks for confirming @ruledio! I'll try to get that merged into master and released later today. |
A quick note Minio doesn't really have regions so you have to set a fake one on the s3 URL
|
closing, since e860748 is now merged. |
Adds minio support via HTTP,